Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addon-docs: Fix style bleeding #21150

Merged

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Feb 17, 2023

Closes #20600

What I did

  • Change the "global" docs styles from being set on the DocsContent component to being actually global. This allows us to wrap them in a CSS Cascade Layer, which has lower specificity than un-layered styles that the user provides. Reverted
  • Change the unstyled logic from targeting .sb-unstyled + known blocks to only targeting .sb-unstyled, and then let the blocks add that class. This simplified the global selector greatly. See initial discussion (Docs: using targetted styles, not components to style MDX #19958 (comment)
  • Include the Anchor block as an unstyled target, as Story and Canvas was wrapped in Anchor in autodocs, so inheritable styles being set on the Anchor would bleed all the way into stories via inheritance. this is what actually fixed the issue.
  • Cleanup some logic and imports to make the flow easier to understand.

How to test

  • See stories in UI Storybook, specifically the Unstyled docs.

Here's an example MDX I wrote that shows how styles no longer bleed into stories. There's a global font-family: Cambria, which has ultra-low specificity but is still being respected in the button. It wasn't previously.

image

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@JReinhold JReinhold linked an issue Feb 17, 2023 that may be closed by this pull request
@JReinhold JReinhold requested a review from shilman February 17, 2023 23:12
@JReinhold JReinhold self-assigned this Feb 17, 2023
@JReinhold JReinhold changed the title Docs: Fix style bleeding Docs: Fix style bleeding DO NOT MERGE Feb 17, 2023
@JReinhold JReinhold changed the title Docs: Fix style bleeding DO NOT MERGE Docs: Fix style bleeding Feb 18, 2023
@JReinhold
Copy link
Contributor Author

JReinhold commented Feb 18, 2023

I have an issue with our UI Storybook here that I'm unsure how to solve.

Basically this PR moves the styles from the DocsPage component to be global instead with Emotion's Global component - so we can encapsulate them in a CSS Cascade Layer. See:
https://github.com/storybookjs/storybook/pull/21150/files#diff-a9dc1d24700cb91a5a432c4f8f92413ee540546c00137eba248a9a44b2d1b4e1R448-R450

The problem is with the stories for any doc block, and the theme switcher. Our theme switcher tool has this "stacked" mode that shows the story in both light and dark theme, by rendering the story twice (see https://github.com/storybookjs/storybook/blob/20600-bug-storybook-docs-css-cascading-into-our-components/code/ui/.storybook/preview.tsx/#L196-L214). But when you render the DocsPage twice, it will set the global styles first with the light theme and then with the dark theme. And because they're global styles it means that both instances will end up with the dark theme because the global styles will override eachother, which makes the stacked view unusable.
The crucial part here is that all our Chromatic snapshots uses this stacked theme.

As an example, see this story in light theme:
https://635781f3500dd2c49e189caf-lxlyyqdibf.chromatic.com/?path=/story/storybook-blocks-blocks-description--of-component-as-component-comment&globals=theme:light
and in stacked theme (scroll):
https://635781f3500dd2c49e189caf-lxlyyqdibf.chromatic.com/?path=/story/storybook-blocks-blocks-description--of-component-as-component-comment&globals=theme:stacked
Notice in the stacked theme, how the text on the light background is barely visible, meaning the text uses the dark theme.

I can think of a few ways to get around this, but none of them are great so I'm hoping someone else will come up with better ideas.

  1. Disable stacked view for any affected stories, and only render (and snapshot) the light theme. This means we won't have dark themed snapshots of doc blocks.
  2. Same as above, but create duplicate stories for light and dark theme so we still have stories and snapshopts for both. This is cumbersome.
  3. Special case stories so they don't use global CSS but still the old scoped CSS. This makes the stories not behave exactly like the components would in a real scenario, I'm not a fan.
  4. Somehow make the stacked theme render stories in iframes or something that would still scope the global CSS independently. I'm not sure if this is even possible.
  5. Drop the global styles and the CSS Cascade Layer. It wasn't what actually fixed the issue here (I believe) and it's only theoretical that it makes it easier to override, I haven't tried to build a use case that proves this has an effect in SB docs.

@tmeasday
Copy link
Member

  1. Drop the global styles and the CSS Cascade Layer.

I don't really have an opinion on this solution but it does seem like a fairly large change to make if you aren't sure it's super beneficial.

Same as above, but create duplicate stories for light and dark theme so we still have stories and snapshopts for both. This is cumbersome.

We probably only need one or two stories per doc block in the dark theme, I'm guessing? So maybe this solution isn't so onerous? The stacked approach is a bit of a hack in any case, I guess a better thing would be for Chromatic to just support multiple themes already ...

@JReinhold
Copy link
Contributor Author

  1. Drop the global styles and the CSS Cascade Layer.

I don't really have an opinion on this solution but it does seem like a fairly large change to make if you aren't sure it's super beneficial.

You're right, let's not overcomplicate things if we don't know that it brings value. I reverted that part.

@shilman shilman changed the title Docs: Fix style bleeding Addon-docs: Fix style bleeding Feb 21, 2023
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't entirely grok this but it seems like an improvement

@JReinhold JReinhold merged commit 1a0c0a8 into next Feb 21, 2023
@JReinhold JReinhold deleted the 20600-bug-storybook-docs-css-cascading-into-our-components branch February 21, 2023 08:27
connor-baer added a commit to sumup-oss/circuit-ui that referenced this pull request Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Storybook docs CSS cascading into our components
3 participants